Ensure cider-jack-in fails gracefully when commands are not found #1733

radhikalism · 2016-05-05T16:21:33Z

Problem: Running cider-jack-in with a Boot project when boot or boot.sh is not found in exec-path yields an error like this:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  locate-file-internal(nil ("/usr/local/bin" "/usr/bin" "/bin" "/usr/local/games" "/usr/games" "/usr/lib/emacs/24.5/x86_64-linux-gnu") ("") 1)
  locate-file(nil ("/usr/local/bin" "/usr/bin" "/bin" "/usr/local/games" "/usr/games" "/usr/lib/emacs/24.5/x86_64-linux-gnu") ("") 1)
  executable-find(nil)
  cider--boot-present-p()
  cider-jack-in(nil)
  call-interactively(cider-jack-in record nil)
  command-execute(cider-jack-in record)
  execute-extended-command(nil "cider-jack-in")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

Expected: a more user-friendly message like this:
The boot executable (specified by cider-lein-command' or cider-boot-command') isn't on your exec-path'`

The source of the bug seems to be that cider-boot-command can be nil since it has a non-literal value, whereas cider--boot-present-p assumes it is non-nil. I've added a fallback literal value "boot" to guard against this causing problems.

Affects cider-20160501.1921 (melpa) and git master.

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

The commits are consistent with our contribution guidelines
You've added tests (if possible) to cover your change(s)
All tests are passing (make test)
The new code is not generating bytecode or M-x checkdoc warnings
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the readme (if adding/changing user-visible functionality)
You've updated the refcard (if you made changes to the commands listed there)

Malabarba · 2016-05-05T17:04:24Z

I like this solution. IIRC, there are situations in which the exec-path might get set after our defcustom is defined, so it's a good idea to have a sane fallback.
@arbscht Please also document this as a bug fix in Changelog.md

bbatsov · 2016-05-05T17:40:14Z

Is there the same problem with lein as well?

bbatsov · 2016-05-05T17:41:44Z

Btw, I guess you've seen the code here, right - https://github.com/clojure-emacs/cider/blob/master/cider.el#L497

radhikalism · 2016-05-05T17:48:35Z

@bbatsov I don't think lein is affected. lein and gradle are defined as literal values, whereas boot is subject to change due to invoking executable-find. Compare:

(defcustom cider-lein-command
  "lein"
...)

(defcustom cider-boot-command
  (or (executable-find "boot")
      (executable-find "boot.sh")) ;; <-- this can be nil
...)

The nil value of cider-boot-command trickles down to cider-present-p and raises an error before the branching in L476 can proceed.

This fix adds a default fallback literal value so that if executable-find returns nil, the cider-present-p check will still evaluate properly and return nil, so that the existing error handling code path in L497 can happen.

bbatsov · 2016-05-05T18:58:18Z

I get what the fix does. I simply dislike we're doing different things for lein, boot and gradle. Maybe we should just remove executable-find. If someone uses boot.sh they'll just update the defcustom. @Malabarba thoughts?

bbatsov · 2016-05-05T19:00:56Z

cider.el

@@ -110,7 +110,8 @@ version from the CIDER package or library.")

 (defcustom cider-boot-command
  (or (executable-find "boot")
-      (executable-find "boot.sh"))
+      (executable-find "boot.sh")
+      "boot")


If we do something like this the first check would be redundant. As I mentioned earlier - I'd rather simplify this to be just a constant to keep in aligned with other similar values.

If not searching for "boot.sh", then there's no need to search for "boot" at this point either. Simply setting "boot" should be enough as executable-find is called later with this value. However, I note that cider--{cmd}-present-p and cider-jack-in-command actually do different things to locate an executable — this seems like a bug.

Malabarba · 2016-05-05T19:09:04Z

Ok. I'm with changing it to just boot.

bbatsov · 2016-05-05T19:58:30Z

One more thing - the real place to fix the bug is the place where a nil can be passed to executable-find in the code I linked to. This should definitely be addressed as well. You can also add a simple unit test.

radhikalism · 2016-05-05T23:34:09Z

I agree with changing the value to just "boot", and let "boot.sh" users simply customize the variable, instead of magically nesting executable-find in multiple layers.

I'm not sure if the main code path needs to protect itself against such nils. It seems like a reasonable assumption that a custom variable is non-nil, given that it is typed 'string and Emacs enforces or interprets the type at set-variable and customize-variable. This is only unreliable in case of initializing the value to nil (as currently the case), but the problem really is that nil was used for a string type custom variable (no fallback).

bbatsov · 2016-05-06T00:14:14Z

Fine. Let's just change the default and wrap this.

On Thursday, 5 May 2016, Abhishek Reddy notifications@github.com wrote:

I agree with changing the value to just "boot", and let "boot.sh" users
simply customize the variable, instead of magically nesting
executable-find in multiple layers.

I'm not sure if the main code path needs to protect itself against such
nils. It seems like a reasonable assumption that a custom variable is
non-nil, given that it is typed 'string and Emacs enforces or interprets
the type at set-variable and customize-variable. This is only unreliable
in case of initializing the value to nil (as currently the case), but the
problem really is that nil was used for a string type custom variable (no
fallback).

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1733 (comment)

Best Regards,
Bozhidar Batsov

http://www.batsov.com

…ns and slightly refactor cider-jack-in

radhikalism · 2016-05-06T00:27:37Z

Here's a somewhat bigger change to normalize behaviour between the present-p check and the command search code paths. This allows custom variables for cider-{lein,boot,gradle}-command to be simple string command names; the resolved location can safely be nil; the error message if a command is not found shows the configured name of the missing command.

Malabarba · 2016-05-06T06:33:59Z

cider.el

+    ("lein" (cider--lein-resolve-command))
+    ("boot" (cider--boot-resolve-command))
+    ("gradle" (cider--gradle-resolve-command))
+    (_ (error "Unsupported project type `%s'" project-type))))


user-error

Ensure cider-boot-command has a non-nil fallback to prevent errors

504f7c0

Add bugfix PR clojure-emacs#1733 to changelog

56802bd

bbatsov reviewed May 5, 2016
View reviewed changes

Unify jack-in-command location-resolution and existence-check functio…

29b5fd4

…ns and slightly refactor cider-jack-in

Malabarba reviewed May 6, 2016
View reviewed changes

Docs and tweaks around jack-in-command resolution code fixes.

92747d1

radhikalism changed the title ~~Ensure cider-boot-command has a non-nil fallback to prevent errors~~ Ensure cider-jack-in fails gracefully when commands are not found May 6, 2016

bbatsov merged commit 775b7d2 into clojure-emacs:master May 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure cider-jack-in fails gracefully when commands are not found #1733

Ensure cider-jack-in fails gracefully when commands are not found #1733

radhikalism commented May 5, 2016 •

edited

Loading

Malabarba commented May 5, 2016

bbatsov commented May 5, 2016

bbatsov commented May 5, 2016

radhikalism commented May 5, 2016 •

edited

Loading

bbatsov commented May 5, 2016

bbatsov May 5, 2016

radhikalism May 5, 2016 •

edited

Loading

Malabarba commented May 5, 2016

bbatsov commented May 5, 2016 •

edited

Loading

radhikalism commented May 5, 2016

bbatsov commented May 6, 2016

radhikalism commented May 6, 2016

Malabarba May 6, 2016

Ensure cider-jack-in fails gracefully when commands are not found #1733

Ensure cider-jack-in fails gracefully when commands are not found #1733

Conversation

radhikalism commented May 5, 2016 • edited Loading

Malabarba commented May 5, 2016

bbatsov commented May 5, 2016

bbatsov commented May 5, 2016

radhikalism commented May 5, 2016 • edited Loading

bbatsov commented May 5, 2016

bbatsov May 5, 2016

Choose a reason for hiding this comment

radhikalism May 5, 2016 • edited Loading

Choose a reason for hiding this comment

Malabarba commented May 5, 2016

bbatsov commented May 5, 2016 • edited Loading

radhikalism commented May 5, 2016

bbatsov commented May 6, 2016

radhikalism commented May 6, 2016

Malabarba May 6, 2016

Choose a reason for hiding this comment

radhikalism commented May 5, 2016 •

edited

Loading

radhikalism commented May 5, 2016 •

edited

Loading

radhikalism May 5, 2016 •

edited

Loading

bbatsov commented May 5, 2016 •

edited

Loading